With sync update, get revision name with latestReadyRevisionName #505

navidshaikh · 2019-11-15T06:36:43Z

/lint

~~Fixes # 500~~ Check #506

Proposed Changes

We got synchronous service update operation, ensuring the requested
config change is reconciled before service update operation returns. feature(service): Wait on update for service to become ready. #271
Traffic splitting e2e tests, do sync service update to generate the
revisions.
With every service update, we need to grab the revision
name generated with this update (for subsequent operations),
which we can grab using latestReadyRevisionName since sync service updated returned.

- We got synchronous service update operation, ensuring the requested config change is reconciled before service update operation returns. knative#271 - Traffic splitting e2e tests, do sync service update to generate the revisions. - With every service update, we need to grab the revision name generated with this update (for subsequent operations), which we can grab using `latestReadyRevisionName` since sync service updated returned.

knative-prow-robot

@navidshaikh: 0 warnings.

In response to this:

/lint

Fixes #500

Proposed Changes

We got synchronous service update operation, ensuring the requested
config change is reconciled before service update operation returns. feature(service): Wait on update for service to become ready. #271

Traffic splitting e2e tests, do sync service update to generate the
revisions.

With every service update, we need to grab the revision
name generated with this update (for subsequent operations),
which we can grab using latestReadyRevisionName since sync service updated returned.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2019-11-15T06:37:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [navidshaikh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

navidshaikh · 2019-11-15T07:00:39Z

Per #500 e2e for a test,
the operations performed are
1 - kn service create [..] <- create a revision
2 - kn service update --env [..] <- create another revision
3 - kn service update --tag <- don't create revision but only update traffic block to append a target with a tag
4 - Expected revision created in step 2 in latestRevision: true block.

actual result:

Revision created in step 1 is set in latestRevision: true block.

The service update returned successfully, means the requested changes were reconciled, then
latestCreatedRevision should be equal to latestReadyRevision.

The failure though reports, latestCreatedRevision != latestReadyRevision

    --- FAIL: TestTrafficSplit/tag_a_revision_as_candidate,_without_otherwise_changing_any_traffic_split (7.74s)
        traffic_split_test.go:368: assertion failed: 
            --- expectedTargets
            +++ formattedActualTargets
              []e2e.TargetFields{
              	{
              		Tag:      "",
            - 		Revision: "echo2-ytdyn-2",
            + 		Revision: "echo2-qjxkx-1",
              		Percent:  100,
              		Latest:   true,
              	},
              	{Tag: "candidate", Revision: "echo2-qjxkx-1"},
              }

which means that, service didn't put correct revisionName for latestReadyRevision:true traffic block.

navidshaikh · 2019-11-15T07:04:08Z

We can update the traffic splitting tests to reference BYO revision names using --revision-name for create and update. This eliminates the need to grab the revision name after last create/update operation performed, this should be another test to see if the revisions names set in traffic block are as expected. Updating the PR..
/hold

navidshaikh · 2019-11-15T07:26:11Z

/test pull-knative-client-integration-tests-latest-release

navidshaikh · 2019-11-15T07:37:23Z

From the first CI run against serving nightly (pull-knative-client-integration-tests ), excerpts logs

Failed to successfully execute 
'kn service update echo1 --traffic echo1-yxfyl-1=20,echo1-yxfyl-1=80 -n kne2etests2':
Execution error: stderr: 'repetition of revision reference echo1-yxfyl-1 is not allowed, 
use only once with --traffic flag

The steps before the failed operation were:
1 - Sync kn service create

2 - Sync kn service update svcName --env [..] - reconciled

3 - rev1 := getLatestReadyRevision (using jsonpath={.status.latestReadyRevisionName})

4 - Sync kn service update svcName --env [..] - reconciled

5 - rev2 := getLatestReadyRevision (using jsonpath={.status.latestReadyRevisionName})

At this point, we expect rev1 != rev2, however per error message getLatestReadyRevision in step 2 and 4 returned same revision.

- for traffic splitting e2e tests - Check the comment on knative#500 (comment)

knative-prow-robot · 2019-11-15T13:55:49Z

@navidshaikh: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-client-integration-tests	`f8da123`	link	`/test pull-knative-client-integration-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

navidshaikh · 2019-11-15T14:48:57Z

Beside pausing for 3 seconds to let latestReadyRevision populate the correct revision name while verifying the targets, we are also getting this issue when we extract the server side generated revision names after each update. So fix should be in #506 where we have BYO revision-names for all the operations, with wait/sleep happening only at one place.

navidshaikh · 2019-11-15T15:37:54Z

The needed changes are addressed in #506
/close

knative-prow-robot · 2019-11-15T15:37:56Z

@navidshaikh: Closed this PR.

In response to this:

The needed changes are addressed in #506
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This complicates somewhat the teardown process, especially that test logger can no longer be valid (`t` might be destroyed already, causing a panic). This logging seems not to contribute much anyway. Also did a pass over the file and cleaned some stuff here and there. Once commited, I'll cleanup serving where it's used.

* [SRVCLI-250] Sync e2e scripts from master * fix: Fix branch names

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 15, 2019

knative-prow-robot requested review from mattmoor and rhuss November 15, 2019 06:36

knative-prow-robot reviewed Nov 15, 2019

View reviewed changes

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 15, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2019

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2019

Pauses 3 seconds before verifying actual vs expected traffic targets

f8da123

- for traffic splitting e2e tests - Check the comment on knative#500 (comment)

knative-prow-robot closed this Nov 15, 2019

dsimansk added a commit to dsimansk/client that referenced this pull request Oct 22, 2020

Sync e2e scripts from master to release-v0.17.2 (knative#505)

2cc2516

* [SRVCLI-250] Sync e2e scripts from master * fix: Fix branch names

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

With sync update, get revision name with latestReadyRevisionName #505

With sync update, get revision name with latestReadyRevisionName #505

navidshaikh commented Nov 15, 2019 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot commented Nov 15, 2019

navidshaikh commented Nov 15, 2019 •

edited

Loading

navidshaikh commented Nov 15, 2019

navidshaikh commented Nov 15, 2019

navidshaikh commented Nov 15, 2019 •

edited

Loading

knative-prow-robot commented Nov 15, 2019

navidshaikh commented Nov 15, 2019 •

edited

Loading

navidshaikh commented Nov 15, 2019

knative-prow-robot commented Nov 15, 2019

With sync update, get revision name with latestReadyRevisionName #505

With sync update, get revision name with latestReadyRevisionName #505

Conversation

navidshaikh commented Nov 15, 2019 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot commented Nov 15, 2019

navidshaikh commented Nov 15, 2019 • edited Loading

navidshaikh commented Nov 15, 2019

navidshaikh commented Nov 15, 2019

navidshaikh commented Nov 15, 2019 • edited Loading

knative-prow-robot commented Nov 15, 2019

navidshaikh commented Nov 15, 2019 • edited Loading

navidshaikh commented Nov 15, 2019

knative-prow-robot commented Nov 15, 2019

navidshaikh commented Nov 15, 2019 •

edited

Loading

navidshaikh commented Nov 15, 2019 •

edited

Loading

navidshaikh commented Nov 15, 2019 •

edited

Loading

navidshaikh commented Nov 15, 2019 •

edited

Loading